Skip to content

Change types to not assume an arch#425

Merged
Reflejo merged 1 commit intoenvoyproxy:masterfrom
Reflejo:fix-types-mismatch
Feb 6, 2017
Merged

Change types to not assume an arch#425
Reflejo merged 1 commit intoenvoyproxy:masterfrom
Reflejo:fix-types-mismatch

Conversation

@Reflejo
Copy link
Contributor

@Reflejo Reflejo commented Feb 4, 2017

Some of these types were assuming that size_t == uint64_t or that uint64_t == unsigned long but that's not universally true, some BSDs such as Darwin define uint64_t as unsigned long long which warns on literals that are defined as UL.

Additionally there was also some assumptions made on the pch as to forward declaration. So I explicitly added some needed ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we keep uint64_t as the var type (prefer to not use auto outside of for loops for clarity)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint64_t: length() returns that and we are trying to make sure it is 64-bit unsigned (prefer to not use size_t for the reasons you are trying to fix).

Copy link
Contributor Author

@Reflejo Reflejo Feb 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah my bad, I followed the wrong length() and thought it was size_t. Will fix

Copy link
Contributor Author

@Reflejo Reflejo Feb 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I think we should change this signature to take uint64_t instead (I mean onDataSourceRead's signature); does that makes sense to you? I'm pushing that change instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry are these changes required because MallocExtension takes size_t as param?

Copy link
Contributor Author

@Reflejo Reflejo Feb 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, since the constraint is on GetNumericProperty I think is better to be explicit here for clarity if that makes sense; also this of course fails on Darwin since __SIZE_TYPE__ is unsigned long and uint64_t is unsigned long long

@Reflejo Reflejo force-pushed the fix-types-mismatch branch from f9175db to 2bea251 Compare February 4, 2017 22:06
@mattklein123 mattklein123 reopened this Feb 6, 2017
@mattklein123
Copy link
Member

@Reflejo please fix format and merge master

Some of these types were assuming that `size_t == uint64_t` or that
`uint64_t == unsigned long` but that's not universally true, some BSDs
such as Darwin define uint64_t as unsigned long long which warns on
literals that are defined as `UL`.

Additionally there were also some assumptions made on the pch as to
forward declarations. So I explicitly added some needed ones.
@Reflejo Reflejo force-pushed the fix-types-mismatch branch from 2bea251 to ace7d21 Compare February 6, 2017 21:58
@Reflejo Reflejo merged commit 5fc9cb9 into envoyproxy:master Feb 6, 2017
@Reflejo Reflejo deleted the fix-types-mismatch branch February 6, 2017 22:34
zuercher pushed a commit that referenced this pull request Jan 23, 2018
…#2387)

Description:
Extend the health check filter to optionally compute its HTTP response status
based on whether at least a specified percentage of servers in a specified set
of upstream clusters are healthy.

Risk Level: Medium

Testing: Unit tests included

Docs Changes: #425

Release Notes: Included in this PR

Fixes: #2362

API Changes: #417

Signed-off-by: Brian Pane bpane@pinterest.com
rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018
Automatic merge from submit-queue.

Introduce new attribute connection.mtls

**What this PR does / why we need it**: Introduce new mixer attribute "connection.mtls" which indicates whether connection is mutual TLS enabled. Add virtual function bool IsMutualTlsEnabledConnection() into http/check_data.h and tcp/check_data.h, and call this function inside AttributesBuilder::ExtractCheckAttributes(). Add another virtual function bool GetDestinationIpPort(std::string* ip, int* port) to report destination IP and port from HTTP filter.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes envoyproxy#426 

**Special notes for your reviewer**:

**Release note**:

```release-note
```
jpsim pushed a commit that referenced this pull request Nov 28, 2022
`Client` -> `HTTPClient`
`Envoy` -> `EnvoyClient`
`EnvoyBuilder` -> `EnvoyClientBuilder`

Consistent with [this Android change](envoyproxy/envoy-mobile#424), and helps us to avoid naming collisions with the `Envoy` type and `Envoy` module namespace.

Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
`Client` -> `HTTPClient`
`Envoy` -> `EnvoyClient`
`EnvoyBuilder` -> `EnvoyClientBuilder`

Consistent with [this Android change](envoyproxy/envoy-mobile#424), and helps us to avoid naming collisions with the `Envoy` type and `Envoy` module namespace.

Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: JP Simard <jp@jpsim.com>
mathetake added a commit that referenced this pull request Mar 3, 2026
**Commit Message**

After #325, the translator is not where the abstraction over :path is
implemented, but it moved to the choice of processor. As a result, the
translator interface has become effectively tied to a specific endpoint,
notably /v1/chat/completion. This renames the `translator.Translator`
interface accordingly not only to remove the unnecessary code path but
also facilitates the implementation of additional features like metrics.

**Related Issues/PRs (if applicable)**

Follow up on #325 and contribute to #316

---------

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants